Fix incorrect assignment of deeply-nested tags to top-level tags #277
Conversation
This test exercises the corner case where a deeply-nested SeriesInstanceUID attribute can be incorrectly saved back to the top-level SeriesInstanceUID attribute, overwriting its value.
In cases where tags are deeply nested within sequences, the output of deid functions were incorrectly being assigned to the top-level tags with matching names. For example, a nested SeriesInstanceUID within a ReferencedSeriesSequence was overwriting the top-level SeriesInstanceUID value. This was resolved by traversing the tree of tags and setting the nested element, rather than the top-level element.
|
@ReeceStevens for black we have it pinned to |
|
Thanks @vsoch! Updated and pushed. |
deid/dicom/parser.py
Outdated
| else: | ||
| return int(tag_string) |
There was a problem hiding this comment.
You can simplify this a bit - if the first statement returns, the else isn't needed.
| else: | |
| return int(tag_string) | |
| return int(tag_string) |
deid/dicom/parser.py
Outdated
| if tag_string.startswith("("): | ||
| result = parentheses_hex_tag_format.match(tag_string) | ||
| return int(f"0x{result.group(1)}{result.group(2)}", base=16) | ||
| else: |
There was a problem hiding this comment.
Same here (per my comment below). If we return in the first statement, then the else here isn't needed, and you can unindent all of the following logic and make it easier for the developer (us!) to read.
deid/dicom/parser.py
Outdated
| # A string representation of the full tag path, including parent sequences | ||
| full_tag_path = field | ||
| if hasattr(field, "uid"): | ||
| full_tag_path = field.uid |
There was a problem hiding this comment.
What is this little section doing?
There was a problem hiding this comment.
I added a comment here for more clarity. This handles the edge case that field can be a string or an object with a uid property containing the tag path.
| else: | ||
| self.dicom.add(element) | ||
| dataset_cursor = self.dicom | ||
| *parent_tags, last_tag = full_tag_path.split("__") |
There was a problem hiding this comment.
Can you add a comment here about how this data is unwrapped, and why the last_tag is special/different?
There was a problem hiding this comment.
Comment added! Let me know if you'd like me to expand on it more.
| @@ -0,0 +1,104 @@ | |||
| import contextlib | |||
|
@vsoch ready! |
|
@vsoch thanks for helping shepherd this through review! Do you know if you'll be able to publish the patch version to pypi some time soon? Wasn't sure if there was a regular release cadence you followed for this. |
|
Thank you for the reminder! |
2 participants
Description
Related issues: I haven't opened an issue for this yet, nor did I see a related issue already open.
Hello pydicom team! Thanks for your excellent work on this library. I ran across an issue in production for which I wanted to push an upstream fix.
During header deidentification, deeply nested tag values (i.e.
ReferencedSeriesSequence > 0 > SeriesInstanceUID) were being processed and saved back to the top-level tag (i.e, just the top-levelSeriesInstanceUID). This was causing some very confusing and surprising behavior with UIDs not being converted to the expected values.I added a minimal reproducible test case (1f9cb7e) which fails pre-patch. Once you apply commit 8b646e5, the test passes.
Please let me know if you have any questions or concerns about the patch! Thanks!
Checklist
Open questions
None